fix(apps): write Deno runtime config to temp directory#41338
Conversation
🦋 Changeset detectedLatest commit: 88dd944 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Looks like this PR is ready to merge! 🎉 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
|
| Layer / File(s) | Summary |
|---|---|
Resolve ephemeral runtime imports packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts |
generateEphemeralDenoConfig accepts packagePath, adds package and base-runtime import mappings, and receives the path from the controller. |
Isolate runtime configuration artifacts packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts, .changeset/tidy-snails-smile.md |
The ephemeral configuration is written as deno.runtime.jsonc inside the temporary directory, with a patch release changeset documenting the startup error fix. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Possibly related PRs
- RocketChat/Rocket.Chat#40947: Modifies the same Deno runtime initialization path and ephemeral configuration setup.
Suggested labels: type: bug
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly describes the main change: moving the Deno runtime config write location to the temp directory. |
| Linked Issues check | ✅ Passed | The runtime now writes ephemeral config under the host temp path, addressing the non-root EACCES startup failure in #41006. |
| Out of Scope Changes check | ✅ Passed | The changes stay focused on the Deno runtime fix and a release notes entry, with no obvious unrelated additions. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
The implementation and regression coverage have been locally verified ( |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #41338 +/- ##
===========================================
- Coverage 68.45% 68.45% -0.01%
===========================================
Files 4092 4092
Lines 158217 158213 -4
Branches 28595 28651 +56
===========================================
- Hits 108315 108306 -9
- Misses 44866 44884 +18
+ Partials 5036 5023 -13
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts (1)
79-85: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid
EPERMon Windows by usingjunctionfor directory symlinks.When running the development environment on Windows,
fs.symlinkSyncwith the'dir'type requires Administrator privileges or Developer Mode. Without them, it will throw anEPERMerror and crash the runtime startup.Changing the type to
'junction'avoids this requirement on Windows and is safely ignored on POSIX platforms (Linux/macOS).♻️ Proposed fix
try { - fs.symlinkSync(path.dirname(this.denoConfigPath), path.dirname(this.denoRuntimePath), 'dir'); + fs.symlinkSync(path.dirname(this.denoConfigPath), path.dirname(this.denoRuntimePath), 'junction'); } catch (reason: unknown) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts` around lines 79 - 85, Update the fs.symlinkSync call in the AppsEngineDenoRuntime startup setup to use the 'junction' directory-link type instead of 'dir', preserving the existing EEXIST handling and rethrow behavior for other errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts`:
- Around line 40-43: Update the import-map target construction in
AppsEngineDenoRuntime to use POSIX/URL-style separators for the
`@rocket.chat/apps-engine/`, `@rocket.chat/apps/base-runtime/`, and
`@rocket.chat/apps/` entries; replace platform-dependent path.join usage with
path.posix.join or pathToFileURL while preserving the existing target locations.
- Around line 71-74: Update the AppsEngineDenoRuntime constructor to create a
unique controller-specific temporary subdirectory beneath
manager.getTempFilePath(), and place deno.runtime.jsonc and the deno-runtime
symlink under that directory. Track the directory for cleanup, removing it in
stopApp() and when constructor initialization fails. Ensure concurrent Deno
controllers never share these paths.
---
Nitpick comments:
In `@packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts`:
- Around line 79-85: Update the fs.symlinkSync call in the AppsEngineDenoRuntime
startup setup to use the 'junction' directory-link type instead of 'dir',
preserving the existing EEXIST handling and rethrow behavior for other errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1cfd7cc3-fc0c-4d2f-8a50-13a90af0f7da
📒 Files selected for processing (2)
.changeset/tidy-snails-smile.mdpackages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
⚠️ CI failures not shown inline (1)
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts
🧠 Learnings (5)
📚 Learning: 2026-03-16T21:50:37.589Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:37.589Z
Learning: For changes related to OpenAPI migrations in Rocket.Chat/OpenAPI, when removing endpoint types and validators from rocket.chat/rest-typings (e.g., UserRegisterParamsPOST, /v1/users.register) document this as a minor changeset (not breaking) per RocketChat/Rocket.Chat-Open-API#150 Rule 7. Note that the endpoint type is re-exposed via a module augmentation .d.ts in the consuming package (e.g., packages/web-ui-registration/src/users-register.d.ts). In reviews, ensure the changeset clearly states: this is a non-breaking change, the major version should not be bumped, and the changeset reflects a minor version bump. Do not treat this as a breaking change during OpenAPI migrations.
Applied to files:
.changeset/tidy-snails-smile.md
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts
📚 Learning: 2026-05-11T21:46:23.471Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40463
File: packages/apps/src/lib/SecureFields.ts:17-19
Timestamp: 2026-05-11T21:46:23.471Z
Learning: In Rocket.Chat’s `packages/apps/tsconfig.json`, TypeScript `"strict"` is set to `false`, which disables strict type-checking (including `noImplicitAny`) for `packages/apps`. When reviewing, do not flag TS7053 (and similar strict-mode indexing/type errors) in files under `packages/apps/src/` that are a consequence of this relaxed strictness—e.g., patterns like indexing an `unknown`/`object` via optional chaining such as `object?.[kSecureFields]`.
Applied to files:
packages/apps/src/server/runtime/deno/AppsEngineDenoRuntime.ts
🔇 Additional comments (1)
.changeset/tidy-snails-smile.md (1)
1-6: LGTM!
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Hi, thanks for your contribution! I've simplified things a bit only, but you were on the right path |
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
d-gubert
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
2 participants
Summary
Closes #41006
Testing
../../node_modules/.bin/tsc -p tsconfig.json --noEmit../../node_modules/.bin/eslint src/server/runtime/deno/AppsEngineDenoRuntime.ts tests/server/runtime/DenoRuntimeSubprocessController.test.tsThe full Deno runtime integration test could not complete locally because the environment has Deno 2.9.2 while the repository pins Deno 2.3.1; its shared startup hook timed out before the regression assertions ran.
Summary by CodeRabbit
EACCESissues.